Fix of shrinking volumes with QCOW2 format by slavkap · Pull Request #5225 · apache/cloudstack

slavkap · 2021-07-20T12:08:27Z

Description

If the volumes are in QCOW2 format, the shrinking will be handled on
the agents' side. There are cases in some storage plugins where the
volumes' format is kept in the DB in QCOW2, but the actual format is raw.
Till the current implementation, this was limiting the plugins to shrink
the volumes. Now this will be handled by the storage plugins.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Bug Severity

How Has This Been Tested?

manual tests with NFS/StorPool primary storages

If the volumes are with QCOW2 format the shrinking will be handled on the agents side. There are cases in some storage plugins where the volumes' format is kept in the DB in QCOW2 but the actual format is raw. Till the current implementation this was limiting the plugins to shrink the volumes. Now this will be handled by the storage plugins

svenvogel · 2021-07-20T21:40:55Z

@slavkap Thanks! let me check, i come back. which version?

shwstppr · 2021-07-22T08:07:25Z

@blueorangutan package

blueorangutan · 2021-07-22T08:08:02Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-07-22T08:33:01Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 627

shwstppr · 2021-07-22T11:50:26Z

@blueorangutan test

blueorangutan · 2021-07-22T11:51:02Z

@shwstppr a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-07-22T23:31:10Z

Trillian test result (tid-1352)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40270 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5225-t1352-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_password_server.py
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 86 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	504.78	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	478.20	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	485.66	test_vpc_redundant.py

svenvogel

Tested with Managed Storage in our Test Environment. LGTM!

svenvogel · 2021-07-23T13:20:58Z

@shwstppr can you check and a do a forward merge to main? we need this in main too.

DennisKonrad

LGTM

shwstppr

LGTM. Reverts #4679
@nvazquez @sureshanaparti okay to merge?

nvazquez · 2021-07-23T17:34:28Z

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java


        } catch (Exception e) {
-            throw new CloudRuntimeException("Exception caught during resize volume operation of volume UUID: " + volume.getUuid(), e);
+            throw new CloudRuntimeException(String.format("Failed to resize volume operation of volume UUID: [%s] due to - %s", volume.getUuid(), e.getMessage()));


Can you include the exception e as the second parameter of CloudRuntimeException please?

nvazquez · 2021-07-23T17:35:59Z

Looks good, I left only a comment so the exception is not swallowed when an error happens. Happy to merge forward into main as well

shwstppr · 2021-07-26T10:51:33Z

server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java


        } catch (Exception e) {
-            throw new CloudRuntimeException("Exception caught during resize volume operation of volume UUID: " + volume.getUuid(), e);
+            throw new CloudRuntimeException(String.format("Failed to resize volume operation of volume UUID: [%s] due to - %s", volume.getUuid(), e));


@slavkap I think @nvazquez meant you keep e.getMessage() there but also pass exception object as second param of the CloudRuntimeException as it has CloudRuntimeException(String message, Throwable th)

Suggested change

throw new CloudRuntimeException(String.format("Failed to resize volume operation of volume UUID: [%s] due to - %s", volume.getUuid(), e));

throw new CloudRuntimeException(String.format("Failed to resize volume operation of volume UUID: [%s] due to - %s", volume.getUuid(), e.getMessage()), e);

@shwstppr, thanks! I've misunderstood the comment (sometimes I'm reading selectively... sorry about that)

Sorry if I wasn't clear @slavkap - yes, I meant what @shwstppr explained - thanks for the fix

NP, @nvazquez, I didn't read your suggestion correctly. Thanks all for testing and code review :)

Will log the exception instead the exception message

nvazquez

Code LGTM

nvazquez · 2021-07-26T11:37:30Z

Merging based on approvals and test results

svenvogel · 2021-07-26T12:23:35Z

@nvazquez do you merge it forward? thanks for that 😄

nvazquez · 2021-07-27T11:04:06Z

@svenvogel sure, it's now included in the main branch as well

svenvogel requested a review from shwstppr July 20, 2021 21:37

svenvogel added component:managed-storage type:bug labels Jul 20, 2021

svenvogel added this to the 4.15.2.0 milestone Jul 20, 2021

svenvogel approved these changes Jul 23, 2021

View reviewed changes

DennisKonrad approved these changes Jul 23, 2021

View reviewed changes

shwstppr approved these changes Jul 23, 2021

View reviewed changes

nvazquez reviewed Jul 23, 2021

View reviewed changes

shwstppr reviewed Jul 26, 2021

View reviewed changes

Addressed @nvazquez suggested change

7cdba5f

Will log the exception instead the exception message

slavkap force-pushed the fix_shrinking_volumes branch from 295bfe0 to 7cdba5f Compare July 26, 2021 11:14

nvazquez approved these changes Jul 26, 2021

View reviewed changes

nvazquez merged commit 446337b into apache:4.15 Jul 26, 2021

	throw new CloudRuntimeException(String.format("Failed to resize volume operation of volume UUID: [%s] due to - %s", volume.getUuid(), e));
	throw new CloudRuntimeException(String.format("Failed to resize volume operation of volume UUID: [%s] due to - %s", volume.getUuid(), e.getMessage()), e);

Conversation

slavkap commented Jul 20, 2021

Description

Types of changes

Bug Severity

How Has This Been Tested?

Uh oh!

svenvogel commented Jul 20, 2021

Uh oh!

shwstppr commented Jul 22, 2021

Uh oh!

blueorangutan commented Jul 22, 2021

Uh oh!

blueorangutan commented Jul 22, 2021

Uh oh!

shwstppr commented Jul 22, 2021

Uh oh!

blueorangutan commented Jul 22, 2021

Uh oh!

blueorangutan commented Jul 22, 2021

Uh oh!

svenvogel left a comment

Choose a reason for hiding this comment

Uh oh!

svenvogel commented Jul 23, 2021

Uh oh!

DennisKonrad left a comment

Choose a reason for hiding this comment

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Jul 23, 2021

Uh oh!

shwstppr Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

slavkap Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

nvazquez Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

slavkap Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Jul 26, 2021

Uh oh!

svenvogel commented Jul 26, 2021

Uh oh!

nvazquez commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nvazquez commented Jul 27, 2021 •

edited

Loading